-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[parsing] Support parsing continuous joint in SDFormat #17956
[parsing] Support parsing continuous joint in SDFormat #17956
Conversation
+(status: do not review) while I work on unit tests. |
e30b19b
to
f709d16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-(status: do not review) +@amcastro-tri for feature review please, thanks.
+(release notes: feature)
Reviewable status: LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @xuchenhan-tri)
multibody/parsing/test/detail_sdf_parser_test.cc
line 838 at r1 (raw file):
prismatic_joint.acceleration_upper_limits(), Vector1d(10))); // Continuous joint
ditto, wouldn't we also like to have a test for "Limitless revolute joint"?
Code quote:
Limitless revolute joint
multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf
line 98 at r1 (raw file):
</inertial> </link> <joint name="continuous_joint" type="continuous">
wouldn't we like to have both tests? one for a revolute joint with no limits and one for a continuous joint? even if they are both an equivalent representation of the same mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@EricCousineau-TRI for platform please.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @xuchenhan-tri)
Previously, amcastro-tri (Alejandro Castro) wrote…
I had the same question when modifying this test -- Do people actually specify revolute joints without limit or is it a stop gap for the lack of support for continuous joint? If the answer is the former, I'll add back revolute joint without limits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @xuchenhan-tri)
multibody/parsing/detail_sdf_parser.cc
line 558 at r1 (raw file):
// Drake marks joints with no limits with ±numeric_limits<double>::infinity() // and therefore we make the change here. // Default position limits to infinities which is the case for continuous
FWIW (pre-existing defect) Seems like line continuations in this files are a bit inconsistent, oops!
multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf
line 98 at r1 (raw file):
Previously, xuchenhan-tri wrote…
I had the same question when modifying this test -- Do people actually specify revolute joints without limit or is it a stop gap for the lack of support for continuous joint? If the answer is the former, I'll add back revolute joint without limits.
Given that this current revision might be reducing test coverage, yeah, I think it's good to have revolute_joint_no_limits
as well (making this PR a pure addition).
f709d16
to
c2c365c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @EricCousineau-TRI)
multibody/parsing/detail_sdf_parser.cc
line 558 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
FWIW (pre-existing defect) Seems like line continuations in this files are a bit inconsistent, oops!
I don't see the inconsistency. I intend the new comment to be a new paragraph. Is this better?
multibody/parsing/test/detail_sdf_parser_test.cc
line 838 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
ditto, wouldn't we also like to have a test for "Limitless revolute joint"?
Done, thanks.
multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf
line 98 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Given that this current revision might be reducing test coverage, yeah, I think it's good to have
revolute_joint_no_limits
as well (making this PR a pure addition).
Gotcha, done, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)
multibody/parsing/detail_sdf_parser.cc
line 558 at r1 (raw file):
Previously, xuchenhan-tri wrote…
I don't see the inconsistency. I intend the new comment to be a new paragraph. Is this better?
Ah, no need to take action - I'm just saying that line continuations + whitespace (hanging indent vs. visual indent) are inconsistent, in addition to some hanging indents not even being correct.
However, that was all preexisting, so not worth defecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)
multibody/parsing/detail_sdf_parser.cc
line 558 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ah, no need to take action - I'm just saying that line continuations + whitespace (hanging indent vs. visual indent) are inconsistent, in addition to some hanging indents not even being correct.
However, that was all preexisting, so not worth defecting.
I see. So not this particular line, gotcha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees amcastro-tri,EricCousineau-TRI(platform) (waiting on @xuchenhan-tri)
Closes #14747.
This change is